Congratulations to this PR and thanks for the credit! As a side note, no need to include any license/acknowledgement from my side (declaring the _get_reader function a non-substantial portion of napari-imc here 😇)

Another side note: this PR seems to be yet another use case of napari/napari#2229, which would resolve the kludge - happy to chat about this anytime

1. Yes, sorry about the make build requiring aicspylibczi... We really need to get that built on an M1 mac somehow... @heeler @toloudis any ideas?

It does build for me locally, not sure why not with pip. I can provide it.

2. I am generally okay with it for now, would love to see better support for these operations in general though. I will ask around for more opinions. Gimme a day or so on this PR.
3. I didn't know that readlif was going to provide access to scene names. Sounds like I may (or you or someone else) may need to make an easy change up-stream to base aicsimageio when that happens. We currently just generate these scene names.

I hacked together a way to get full scene names in readlif and it's been working fine. But now there's a readlif branch which provides full scene names. It also works fine with current AICSImageIO. Here's the GitHub issue and post:
Arcadia-Science/readlif#28 (comment)

4. I think if you write a test that simply checks for the existance of the widget I would be happy with that for now personally. Or even better, checking the contents of the widget. I.e. Given a file, check the widget exists and check that it has 3 text rows with strings "1 -- foo", "2 -- bar", "3 -- baz".

I think this should be possible. But I also thought the plugin/widget bit would be easy 🤣
I'm not familiar per se with testing and writing tests, this is really my first time with something like this. (Again, thanks for your contributing guide and encouragement on twitter).

Regarding the other comments:
Obviously I need to familiarize with pytest and tox. I think should be able to get the linting stuff fixed via VS Code settings—it has some built in stuff, black.
I've no idea on the pyside bit—I think it's because qtpy can use both pyqt and pyside? I only have pyqt5 locally.
I'll work my way through your code comments. Thank you for your time.

OK, Noob question: how do I go about making edits now? Do I checkout this PR?
I've done that with the napari repo to test things:
gh pr checkout 25
Or do I work on my fork and then somehow re-PR?

It does build for me locally, not sure why not with pip. I can provide it.

This is just because we don't build and release a version for M1 macs on PyPI because we don't have access to M1 machines for building I believe.

I hacked together a way to get full scene names in readlif and it's been working fine. But now there's a readlif branch which provides full scene names. It also works fine with current AICSImageIO. Here's the GitHub issue and post:
Arcadia-Science/readlif#28 (comment)

Good to know it will just slot in to aicsimageio when ready then 👍

I think this should be possible. But I also thought the plugin/widget bit would be easy rofl
I'm not familiar per se with testing and writing tests, this is really my first time with something like this.

First off, really, thank you! Super happy to both see this in action and to see contribution. Second, a way to test this may be to return the widget from the function, obviously you don't need to do anything with it anywhere else, but if you return the widget, you can write a test that calls the function then checks the contents. OR, the "better" practice here would be to split up the "list of scene id text generation and tuple of scene id text and data" from the qt widget code. In fact I might propose factoring it out that way. I.e.

def get_all_scene_datas(img: AICSImage, in_memory: bool) -> List[Tuple[str, xr.DataArray]]:
    def open_scene(index: int) -> xr.DataArray:
        # your existing function here
        return data

    scene_and_data = []
    for i, scene in enumerate(img.scenes):
        scene_and_data.append((f"{i} -- {scene}", open_scene(index=i))

    return scene_and_data

# then have a function that just uses this list of tuples

Basically split up the "naming and data getter" from the QT viewer stuff. That way you can just test the naming and data stuff!

OK, Noob question: how do I go about making edits now? Do I checkout this PR?
I've done that with the napari repo to test things:
gh pr checkout 25
Or do I work on my fork and then somehow re-PR?

No worries 🙂 You can actually keep working on your original branch and just push new commits. This PR will automatically pull in the changes.

3. It seems that the tests now require PySide.... Don't really know how that happened but could you add PySide to the dependencies? Or I think updating our napari dependency to napari[pyside] should do the trick.

Ok, this error is related to the QtWidget. The requirements:

I've got pytest working locally, in VS Code—it's pretty slick!
Seeing errors with the LIF file:

E           AssertionError: assert {'channel_axi...311154598827)} == {'channel_axi...719055966396)}
E             Omitting 2 identical items, use -vv to show
E             Differing items:
E             {'scale': (0.20061311154598827, 0.20061311154598827)} != {'scale': (4.984719055966396, 4.984719055966396)}
E             Use -v to get the full diff

I'm 99% sure this is due me having AICSImageIO 4.1.0 where you fixed the LIF scale issue:
AllenCellModeling/aicsimageio#287
Because the differences in values is exactly the reciprocal.
As far as I know, Napari uses length per pixel as the scale. So the 512 px tiles at 4.98 scale would be ~2500 micron FOV with a 63X obj, which is impossible 😄
Using 0.2, it yields ~100 micron FOV which is plausible. So I think this test needs to be fixed.
I'll be on the microscope later today, so I will confirm what LAS X shows for that LIF.

Edit: LAS X confirms it's the test that is wrong.

Edit2: I've gotten a test working that checks for the presence of a widget for single and multi scene test files. If the length is not 0, then it confirms the widget name. I'll try to actually load an image layer to verify dtype, shape—hopefully this evening.

Ahhh yes! Thanks for updating the Lif checks. Those were out-of-date.

This is just because we don't build and release a version for M1 macs on PyPI because we don't have access to M1 machines for building I believe.

I don't know how it's built, but in pricinple you don't need an M1 Mac. arm64 and universal binaries can be built cross-architecture, at least using cmake:
https://forum.image.sc/t/fiji-clij-etc-native-on-apple-silicon-arm64-m1/53627/2?u=psobolewskiphd
Of course it's hard to test without M1.
I'm willing to help either way.

Codecov Report

Merging #25 (dacc272) into main (8f298c6) will increase coverage by 4.59%.
The diff coverage is 91.25%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   74.54%   79.14%   +4.59%     
==========================================
  Files           7        7              
  Lines         110      163      +53     
==========================================
+ Hits           82      129      +47     
- Misses         28       34       +6     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f298c6...dacc272. Read the comment docs.

Thanks to @tlambert03 catching my bug, the test for selecting a scene from the widget should be working now. Passes for me locally anyways!

Thanks to @tlambert03 catching my bug, the test for selecting a scene from the widget should be working now. Passes for me locally anyways!

Yep. Looks like there is just lint errors left.

I've been working on the lint errors. I'm not sure how to deal with this one:

napari_aicsimageio/core.py:155: error: List item 0 has incompatible type "Tuple[None]"; expected "Tuple[Any, Dict[str, Any], str]"

This is related to the return [(None,)] empty LayerData. I tried changing it to include the Dict and string, but that resulted in errors. Based on this PR: napari/napari#2206 my understanding is that return [(None,)] is actually correct here, so I'm not sure how to proceed.

Likewise, not sure what to do here:

napari_aicsimageio/tests/test_core.py:117: error: Function is missing a type annotation for one or more arguments

I think this is regarding:

def test_for_multiscene_widget(
    make_napari_viewer,

But I'm not sure what I should put here. make_napari_viewer is a fixture, but that gives an error. Looking in napari tests I've not been able to find a type for it.
The others I think I fixed—the last test ones by copying your # type: ignore by analogy from your test_reader function—hope that's OK.

List item 0 has incompatible type "Tuple[None]"

this is because the LayerData alias at the top of the file is a little out of date (it doesn't include the None, sentinel). Change the defs at the top of the file to be this:

# put this in the imports:  from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from napari.types import LayerData, ReaderFunction, PathLike

make_napari_viewer is a fixture, but that gives an error

make_napari_viewer: Callable[..., napari.Viewer]

Am I doing this right?

from functools import partial
from typing import Any, Callable, Dict, List, Optional, Tuple, Union, TYPE_CHECKING

import napari
import xarray as xr
from aicsimageio import AICSImage, exceptions, types
from aicsimageio.dimensions import DimensionNames
from qtpy.QtWidgets import QListWidget

###############################################################################

if TYPE_CHECKING:
    from napari.types import LayerData, ReaderFunction, PathLike

###############################################################################

Edit2: Nope! That broke everything.
ValueError: No plugin found capable of reading '/Users/piotrsobolewski/Documents/Leica DMi8/2021040_H33342_L929_24w_A3_tilescan.lif'.

Edit3: Wowza, after reverting everything stayed broken, even when using a different conda env. Ended up uninstalling/reinstalling the plugin!

Edit: for this one make_napari_viewer: Callable[..., napari.Viewer] do I also need to import napari in the imports?

for this one make_napari_viewer: Callable[..., napari.Viewer] do I also need to import napari in the imports?

Yep!

Hah. Oof. Give those:

isort napari_aicsimageio/
black napari_aicsimageio/
flake8 napari_aicsimageio/

commands a run.

Looks like the only things failing are just ubuntu tests which is somewhat expected because the ubuntu machines probably have odd display settings. I would be okay with adding a skipif the platform is linux.

for the scene widget tests add something like:

import platform
@pytest.mark.skipif(platform.system() == "Linux", reason="Ubuntu (Linux) runners abort for display tests")

unless @tlambert03 has recommendations on how to get display tests working on GH Actions?

Note the import platform should be with the rest of the imports and isort should validate import order.

unless @tlambert03 has recommendations on how to get display tests working on GH Actions?

try changing the corresponding steps in the tests here to this:

    steps:
    
    # add this step somewhere
    - uses: tlambert03/setup-qt-libs@v1 
  
   # then use xvfb to run the test
    - name: Test with pytest
      uses: GabrielBB/xvfb-action@v1
      with:
        run: pytest --cov-report xml --cov=napari_aicsimageio napari_aicsimageio/tests/

        

Hah. Oof. Give those:

isort napari_aicsimageio/
black napari_aicsimageio/
flake8 napari_aicsimageio/

commands a run.

maybe just use pre-commit. You could get the initial configuration from napari https://github.com/napari/napari/blob/main/.pre-commit-config.yaml

Hey @psobolewskiPhD mind if I finish up this PR today? I believe I can push to your branch, and if not, I will clone your branch and work off of it. Hoping to get this in and release a new version today 👍

@JacksonMaxfield Sure. I was looking at doing this tonight:
#25 (comment)
Which would mean Talley's option 2 would work. Seemed the most elegant and safe long term.

I think Talley's suggestion for the tests on Linux seems worth giving a whirl too:
#25 (comment)

So I was going to do that as well—after the type stuff, so it could be easily reverted.

But you're more then welcome to take the reins—I need to eat some diner and will then have time. With luck, it's quick, but you've seen my work to know better 🤣

@JacksonMaxfield Sure. I was looking at doing this tonight:
#25 (comment)
Which would mean Talley's option 2 would work. Seemed the most elegant and safe long term.

I think Talley's suggestion for the tests on Linux seems worth giving a whirl too:
#25 (comment)

So I was going to do that as well—after the type stuff, so it could be easily reverted.

But you're more then welcome to take the reins—I need to eat some diner and will then have time. With luck, it's quick, but you've seen my work to know better rofl

Sounds good. Take the night off. Hopefully by tomorrow you should see a new release of napari-aicsimageio with this bundled in 🙂

The type checking change from @Czaki plus option 2 from Talley work on my end.
(Pizza in oven, negroni in hand)
The Linux testing change I can't test anyways, so that ball has to be in your court I think.
I'll be putzing with something this evening, so if I can test or help with something let me know 😄

Edit: I remembered to run isort so that should be ok too! Still need to get VS Code to handle that like it does black

(Pizza in oven, negroni in hand)

❤️

My proudest achievement:

Closing as I just merged #27